Handle unknown fields part of a oneof #324

sigurdm · 2019-11-15T12:08:50Z

No description provided.

sigurdm · 2019-11-15T12:20:17Z

Companion CL https://dart-review.googlesource.com/c/sdk/+/125345 has tests.

szakarias

LGTM

protobuf/lib/src/protobuf/field_set.dart

mkustermann · 2019-11-15T17:49:04Z

protobuf/lib/src/protobuf/field_set.dart

@@ -265,8 +265,9 @@ class _FieldSet {
      }
    }

-    // neither a regular field nor an extension.
-    // TODO(skybrian) throw?
+    if (_hasUnknownFields) {


Is it intentional that _clearField(tagNumber) checks unknown field set, but e.g. _hasField(tagNumber) does not?

Good question!
I think we could change the _hasField semantics also.
Done

Since we are now changing semantics to include unknowns, I agree that it is inconsistent if we don't also update _hasField. Good catch!

mkustermann · 2019-11-15T18:04:37Z

protobuf/lib/src/protobuf/field_set.dart

    if (oneofIndex != null) {
      _clearField(_oneofCases[oneofIndex]);
-      _oneofCases[oneofIndex] = tag;
+      _oneofCases[oneofIndex] = newTagnumber;


Maybe only call it conditionally

final currentTag = _oneofCases[oneofIndex]; if (currentTag != null) _clearField(currentTag);

Yes - that's better

mkustermann · 2019-11-15T18:12:18Z

protobuf/lib/src/protobuf/generated_message.dart

+  /// will be kept.
+  ///
+  /// If the tagNumber is present as an unknown field, that field will be
+  /// removed.


GeneratedMessage.mergeFromMessage() to call _FieldSet._mergeFromMessage(), which looks like this:

void _mergeFromMessage(_FieldSet other) { for (FieldInfo fi in other._infosSortedByTag) { var value = other._values[fi.index]; if (value != null) _mergeField(fi, value, isExtension: false); } if (other._hasExtensions) { var others = other._extensions; for (int tagNumber in others._tagNumbers) { var extension = others._getInfoOrNull(tagNumber); var value = others._getFieldOrNull(extension); _mergeField(extension, value, isExtension: true); } } if (other._hasUnknownFields) { _ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields); } }

This merging does not seem to account for oneofs. Does it need to be updated as well? Are there more places?

_FieldSet._mergeFromMessage() calls _mergeField that ends up calling _setNonExtensionFieldUnchecked that handles the oneofs

Maybe my comment was not clear, what I meant is the following:

We seem to have an invariant now

there is at most one of the oneof cases set (either a) it's set in field set b) it's set in unknown field set c) none are set)

whenever we add a new field value, we are guaranteed to clear the old one if one exists.

This invariant is violated because of this line:

if (other._hasUnknownFields) { _ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields); }

We might have already a oneof candidate in the fieldset (or the unknown fieldset). Yet this code blindly copies other._unknownFields, which possibly has a new oneof value, but we do not clear the old one.

Ah - now I get it - thanks! I updated the merging of unknown fields to go field by field, and also update the one-ofs.

Good catch, Martin. Thanks.

sigurdm

PTAL

sigurdm · 2019-11-21T13:13:48Z

protobuf/lib/src/protobuf/field_set.dart

    if (oneofIndex != null) {
      _clearField(_oneofCases[oneofIndex]);
-      _oneofCases[oneofIndex] = tag;
+      _oneofCases[oneofIndex] = newTagnumber;


Yes - that's better

sigurdm · 2019-11-21T13:32:17Z

protobuf/lib/src/protobuf/generated_message.dart

+  /// will be kept.
+  ///
+  /// If the tagNumber is present as an unknown field, that field will be
+  /// removed.


_FieldSet._mergeFromMessage() calls _mergeField that ends up calling _setNonExtensionFieldUnchecked that handles the oneofs

sigurdm · 2019-11-21T13:55:23Z

protobuf/lib/src/protobuf/field_set.dart

@@ -265,8 +265,9 @@ class _FieldSet {
      }
    }

-    // neither a regular field nor an extension.
-    // TODO(skybrian) throw?
+    if (_hasUnknownFields) {


Good question!
I think we could change the _hasField semantics also.
Done

protobuf/lib/src/protobuf/extension_field_set.dart

protoc_plugin/test/oneof_test.dart

sigurdm · 2019-11-22T11:16:03Z

I have also improved the _clearField methods handling of oneofs. It was doing the work twice, and only for known fields @szakarias Could you look over that?

mkustermann

Let Sarah do a careful review (she's probably more familiar with the code).

We should also consider having more unit tests for these brittle cases.

szakarias

LGTM

osa1 · 2022-05-19T07:48:51Z

@sigurdm it seems like the changes to _FieldSet._hasField are separate from the issues with oneof and can be merged separately, with the test you added. Should we do that?

sigurdm · 2022-05-20T08:39:35Z

Yeah I think splitting is fine if it simplifies things

Handle unknown fields part of a oneof

a1cfc04

sigurdm requested review from szakarias and mkustermann November 15, 2019 12:08

googlebot added the cla: yes label Nov 15, 2019

szakarias approved these changes Nov 15, 2019

View reviewed changes

protobuf/lib/src/protobuf/field_set.dart Show resolved Hide resolved

Use helper from _

2e9934d

mkustermann reviewed Nov 15, 2019

View reviewed changes

Address review

d53a9fb

sigurdm commented Nov 21, 2019

View reviewed changes

szakarias reviewed Nov 21, 2019

View reviewed changes

protobuf/lib/src/protobuf/extension_field_set.dart Outdated Show resolved Hide resolved

szakarias reviewed Nov 21, 2019

View reviewed changes

protoc_plugin/test/oneof_test.dart Outdated Show resolved Hide resolved

Revert unrelated changes

5f66ac0

szakarias approved these changes Nov 21, 2019

View reviewed changes

sigurdm added 2 commits November 21, 2019 15:19

Merge to master

2f7be7e

Fix merging unknown fields

309d4cf

mkustermann approved these changes Nov 22, 2019

View reviewed changes

szakarias approved these changes Nov 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle unknown fields part of a oneof #324

Handle unknown fields part of a oneof #324

sigurdm commented Nov 15, 2019

sigurdm commented Nov 15, 2019

szakarias left a comment

mkustermann Nov 15, 2019

sigurdm Nov 21, 2019

szakarias Nov 21, 2019

mkustermann Nov 15, 2019

sigurdm Nov 21, 2019

mkustermann Nov 15, 2019

sigurdm Nov 21, 2019

mkustermann Nov 22, 2019 •

edited

Loading

sigurdm Nov 22, 2019

szakarias Nov 22, 2019

sigurdm left a comment

sigurdm Nov 21, 2019

sigurdm Nov 21, 2019

sigurdm Nov 21, 2019

sigurdm commented Nov 22, 2019

mkustermann left a comment

szakarias left a comment

osa1 commented May 19, 2022

sigurdm commented May 20, 2022

Handle unknown fields part of a oneof #324

Are you sure you want to change the base?

Handle unknown fields part of a oneof #324

Conversation

sigurdm commented Nov 15, 2019

sigurdm commented Nov 15, 2019

szakarias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkustermann Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigurdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigurdm commented Nov 22, 2019

mkustermann left a comment

Choose a reason for hiding this comment

szakarias left a comment

Choose a reason for hiding this comment

osa1 commented May 19, 2022

sigurdm commented May 20, 2022

mkustermann Nov 22, 2019 •

edited

Loading